fix: stop leaking prePromptMessageCount to daemon in afterTurn#1
fix: stop leaking prePromptMessageCount to daemon in afterTurn#1xDarkicex wants to merge 1 commit intofuller-stack-dev:mainfrom
Conversation
The `...args` spread forwarded every framework-provided field to the daemon — prePromptMessageCount, tokenBudget, runtimeContext. The daemon uses prePromptMessageCount to skip messages by array position, and a stale value from the framework silently drops messages before persistence. The daemon already has content-hash dedup (afterTurnIngestedKeys), so the positional prePromptMessageCount hint is redundant for correctness. Strip it from both the kernel and RPC paths and use explicit params instead of the spread. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/integration/host-flow.test.ts (1)
460-473: Cover the full leaked-field set in this regression test.This only proves
prePromptMessageCountis stripped. The old...argsspread also leakedtokenBudgetandruntimeContext, so it would be better to pass both here and assert they stay out ofafter_turn_kernelas well.Suggested test tightening
await context.afterTurn({ sessionId: "test-session", userId: "test-user", messages: mockMessages, prePromptMessageCount: 2, isHeartbeat: false, + tokenBudget: 1024, + runtimeContext: { currentTokenCount: 900 }, }); const params = rpc.getLastCall("after_turn_kernel"); assert.ok(params, "Expected after_turn_kernel to be called"); assert.equal(params.sessionId, "test-session"); assert.equal(params.userId, "test-user"); assert.equal("prePromptMessageCount" in params, false, "prePromptMessageCount must not leak to daemon — daemon defaults to 0 and uses content-hash dedup"); + assert.equal("tokenBudget" in params, false, "tokenBudget must stay host-local"); + assert.equal("runtimeContext" in params, false, "runtimeContext must stay host-local"); assert.equal(params.isHeartbeat, false); assert.deepEqual(params.messages, mockMessages);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/host-flow.test.ts` around lines 460 - 473, The test currently only asserts that prePromptMessageCount is stripped from the params passed to after_turn_kernel; extend it to also verify that tokenBudget and runtimeContext are not leaked: after calling context.afterTurn(...) and retrieving params via rpc.getLastCall("after_turn_kernel"), add assertions that "tokenBudget" and "runtimeContext" are not present on params (e.g., assert.equal("tokenBudget" in params, false) and assert.equal("runtimeContext" in params, false)) while keeping the existing checks for sessionId, userId, and isHeartbeat so the test ensures the full leaked-field set (prePromptMessageCount, tokenBudget, runtimeContext) is stripped before the RPC call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/integration/host-flow.test.ts`:
- Around line 460-473: The test currently only asserts that
prePromptMessageCount is stripped from the params passed to after_turn_kernel;
extend it to also verify that tokenBudget and runtimeContext are not leaked:
after calling context.afterTurn(...) and retrieving params via
rpc.getLastCall("after_turn_kernel"), add assertions that "tokenBudget" and
"runtimeContext" are not present on params (e.g., assert.equal("tokenBudget" in
params, false) and assert.equal("runtimeContext" in params, false)) while
keeping the existing checks for sessionId, userId, and isHeartbeat so the test
ensures the full leaked-field set (prePromptMessageCount, tokenBudget,
runtimeContext) is stripped before the RPC call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e7e085a-892e-42ac-93df-049839d762b4
📒 Files selected for processing (2)
src/context-engine.tstest/integration/host-flow.test.ts
Summary
afterTurnwas spreading...argsinto the RPC call, leakingprePromptMessageCount,tokenBudget, andruntimeContextto the daemonprePromptMessageCountto skip messages by array position — if the framework passes a stale value, messages are silently dropped before persistenceafterTurnIngestedKeys), so the positional skip is redundant for correctnessFix
prePromptMessageCountforwarding...argsspread with explicit{ sessionId, sessionKey, userId, messages, isHeartbeat }prePromptMessageCountto 0, processing all messages; content-hash dedup catches any already-ingestedTest plan
npm run test:integration— all 30 passnpm run test:ts— 72 pass, 10 pre-existing failures (unrelated, present on clean main)Summary by CodeRabbit